HADOOP-18708: Support S3 Client Side Encryption(CSE) With AWS SDK V2 #6884

shameersss1 · 2024-06-12T10:19:31Z

Description of PR

This commit adds support for S3 client side encryption (CSE). CSE can configured in two modes CSE-KMS where keys are provided by AWS KMS and CSE-CUSTOM where custom keys are provided by implementing custom keyring

CSE is implemented using S3EncryptionClient (V3 client) and additional configurations (mentioned below) were added to make it compatible with the older encryption client V1 and V2 which is turned OFF by default.

Inorder to have compatibility with V1 client the following operations are done.

V1 client pads extra bytes in multiple of 16 i.e if the file size is 12 bytes, 4bytes are padded to make it multiple of 16. Inorder to get the unencrypted file size of such S3 object ranged S3 GET call is made
Unlike V1/V2 client V3 client does not support reading unencrypted object, Additional s3 client (base client) is introduced to read mix of encrypted and unencrypted s3 objects.

Default Behavior

The configurations to make it backward compatible is turned OFF by default considering the performance implications. The default behavior is as follows

The unencrypted file size is computed by simply subtracting 16 bytes from the file size.
When there is a mix of unencrypted and encrypted s3 objects, The client fails.

This PR is based on the initial work done by @ahmarsuhail as part of #6164

How was this patch tested?

Tested in us-east-1 with mvn -Dparallel-tests -DtestsThreadCount=16 clean verify.
Added integration test for CSE-KMS and CSE-CUSTOM

shameersss1 · 2024-06-13T05:03:56Z

@steveloughran @ahmarsuhail Could you please review the changes ?

steveloughran

sorry, no time to review right now, vector IO problems

I don't want another s3 client as it will only hurt boot time. are already hitting problems here where client init time is O(jar) & I'm thinking of lazy creation of the async one.

look at how we did this before: we altered file lengths in listings.

no to netty dependencies
no to any dependency on the new library except when CSE is enabled.

hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AFileSystem.java

hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AUtils.java

steveloughran · 2024-06-14T16:33:02Z

FYI, Jira on create performance. I'm worrying about how to remove/delay client configuration, so adding a new client makes things way worse: HADOOP-19205

shameersss1 · 2024-06-14T17:15:00Z

FYI, Jira on create performance. I'm worrying about how to remove/delay client configuration, so adding a new client makes things way worse: HADOOP-19205

by default we are not initializing new s3 client. It is done under a config to provide backward compatibility only when required.

steveloughran · 2024-06-17T09:53:51Z

It's still happening in FileSystem.initialize().

If I get time this week I'll do my PoC of lazy creation of transfer manager and async client, as that's doubled startup time already. all of that will be moved behind S3Store with only the copy() methods exposed.

Ultimately I want the S3Client itself hidden behind that API, so here

new unencryptedS3 operations getLengthOfEncryptedObject(path), ..., would trigger demand creation of the method
accessor to this passed down.

Actually, createFileStatusFromListingEntry() could be part of S3AStore took, somehow. all the config information for the creation, especially cSE flags, would be in the store config, so listing wouldn't need it

steveloughran · 2024-06-18T16:50:47Z

#6892 is the draft pr with lazy creation; a new s3 client would go in there, but ideally we'd actually push the listing/transformation code into S3AStore so the extra client would never be visible outside it.

shameersss1 · 2024-06-19T05:44:27Z

#6892 is the draft pr with lazy creation; a new s3 client would go in there, but ideally we'd actually push the listing/transformation code into S3AStore so the extra client would never be visible outside it.

@steveloughran - I really like the idea of lazy initializing async client and new interface for creating s3 client. I will wait for your changes to get in and refactor my code based on it. By this way i could lazy initialize unencryted s3 client as well when the config is enabled.

steveloughran · 2024-06-19T11:02:22Z

exactly. when its not needed: no penalty.

shameersss1 · 2024-07-08T07:38:14Z

@steveloughran @ahmarsuhail I have rebased the changes on top of #6892 .
Please review the changes

Thanks

steveloughran

(intellij seems to be hiding or has lost some of the review comments...let me see once this review is submitted
)
I'm afraid one thing I now want changed across the entire code base is to stop passing s3 client instances around, so that we can move in future to using the AsyncClient everywhere. The S3AStoreImpl class would provide accessor methods to all operations needed, s3afs would invoke them, while subsidiary components/classes would invoke S3Astore via callbacks which would never call to S3AFS. See BulkDeleteOperationCallbacksImpl for an example.

As well as delivering the alleged speed ups of the async client, the isolation should help testing, maintenance etc. My ultimate goal would be for the S3AFS class to to be a lot more minimal, with no Callback ever been implemented in it.

Anyway, that is a big undertaking. For now: no new accessors of s3client. And no network IO operations in S3AUtils, which should all ready be small utility methods (which is also why error handling is moving to )

Proposed:

ListingOperationCallbacks to provide a createFileStatus() call to create a file status
different implementations/branches of that call to eiher the S3AUtils class, or something which does the extra IO

this is going to have to be pulled out into listing callbacks, executed within the same auditspan and updating statistics. if a client is doing any IO we need to know who and why (server side) and how long (client)

hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/impl/AWSHeaders.java

hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/impl/ClientManagerImpl.java

hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/impl/CSEMaterials.java

hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/impl/ClientManagerImpl.java

hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/impl/CSEUtils.java

hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/impl/HeaderProcessing.java

hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/impl/ClientManagerImpl.java

hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AUtils.java

shameersss1 · 2024-07-13T06:13:45Z

@steveloughran I really appreciate your time and effort to review this changes. Indeed it was a detailed review.
I have tried to address all your comments and raised a new revision (I hope i have covered all your comments).

Please take a look

Thanks

steveloughran · 2024-07-15T13:40:28Z

@shameersss1 unless you have no choice, please try not to rebase and force push prs once reviews have started. makes it a lot harder to see what has changed

raphaelazzolini · 2024-07-17T22:44:44Z

hadoop-tools/hadoop-aws/src/site/markdown/tools/hadoop-aws/encryption.md

@@ -727,6 +736,28 @@ S3-CSE to work.
 </property>
 ```

+#### 2. CSE-CUSTOM
+- Set `fs.s3a.encryption.algorithm=CSE-CUSTOM`.
+- Set `fs.s3a.encryption.cse.custom.cryptographic.material.manager.class.name=<fully qualified class name>`.


Should the new fs.s3a.encryption.cse properties be added to core-default.xml and index.md?

These configurations will be shown as part of the encryption page. similar to https://hadoop.apache.org/docs/r3.4.0/hadoop-aws/tools/hadoop-aws/encryption.html

shameersss1 · 2024-07-22T04:22:52Z

@shameersss1 unless you have no choice, please try not to rebase and force push prs once reviews have started. makes it a lot harder to see what has changed

Yeah, I had to rebase for merge conflicts in hadoop-common module. This commit (838dc24) contains the addressing of review comments.

raphaelazzolini

I think that there is a little bit of complexity in S3AFileSystem with if (isCSEEnabled) conditions in many places. I wonder if, instead of repeating the boolean checks all over the code, we could have a handler interface and extract the code to two implementations: one for isCSEEnabled and one for no CSE.

Other than that, the change looks good to me.

shameersss1 · 2024-07-28T06:45:08Z

@steveloughran - Gentle reminder for the review
Thanks

steveloughran · 2024-08-01T10:40:44Z

@shameersss1 what do you think of @raphaelazzolini 's comment

steveloughran · 2024-10-29T17:55:07Z

Thinking about this some more: those .instruction files raise a lot of problems; I don't like the way t

There's already the notion that files beginning with . or _ are hidden, applications scanning directo

So do we really need to hide them at all? Making them visible stops us playing yet more tricks with the

What is actually in the files? Is it just some list of attributes encrypted with the same CSE settings?

shameersss1 · 2024-10-30T10:03:39Z

Thinking about this some more: those .instruction files raise a lot of problems; I don't like the way t

There's already the notion that files beginning with . or _ are hidden, applications scanning directo

So do we really need to hide them at all? Making them visible stops us playing yet more tricks with the

What is actually in the files? Is it just some list of attributes encrypted with the same CSE settings?

Instruction file contains metadata about the encryption and some headers.
If a file named "abc" is written to s3 with CSE V1 with Instruction mode, It will store two files, "abc" and "abc.instruction"
So we are ultimately skipping all files ending .instruction when that flag is enabled.
I understand your concern, skipping .instruction files will lead to not deleting them when delete or rename operation is called.

Just thinking if there is any better way to solve this. or may be we can simply quote ".instruction" files are not supported and should not be present. @steveloughran anythoughts on this ?

shameersss1 · 2024-10-31T09:56:17Z

Force pushed to rebase with the trunk.

@steveloughran - Thanks a lot for a detailed review. I know it is a pain to review changes which are big (~50 files). I really appreciate the time you put into this.

I have addressed your comments with the new commit

PS: Ignored instruction file handling for simplicilty

hadoop-yetus · 2024-10-31T14:27:25Z

💔 -1 overall

Vote	Subsystem	Runtime	Logfile	Comment
+0 🆗	reexec	18m 8s		Docker mode activated.
			_ Prechecks _
+1 💚	dupname	0m 1s		No case conflicting files found.
+0 🆗	codespell	0m 0s		codespell was not available.
+0 🆗	detsecrets	0m 0s		detect-secrets was not available.
+0 🆗	xmllint	0m 0s		xmllint was not available.
+0 🆗	markdownlint	0m 0s		markdownlint was not available.
+1 💚	@author	0m 0s		The patch does not contain any @author tags.
+1 💚	test4tests	0m 0s		The patch appears to include 17 new or modified test files.
			_ trunk Compile Tests _
+0 🆗	mvndep	14m 41s		Maven dependency ordering for branch
+1 💚	mvninstall	34m 38s		trunk passed
+1 💚	compile	17m 26s		trunk passed with JDK Ubuntu-11.0.24+8-post-Ubuntu-1ubuntu320.04
+1 💚	compile	15m 59s		trunk passed with JDK Private Build-1.8.0_422-8u422-b05-1~20.04-b05
+1 💚	checkstyle	4m 19s		trunk passed
+1 💚	mvnsite	3m 24s		trunk passed
+1 💚	javadoc	2m 51s		trunk passed with JDK Ubuntu-11.0.24+8-post-Ubuntu-1ubuntu320.04
+1 💚	javadoc	2m 24s		trunk passed with JDK Private Build-1.8.0_422-8u422-b05-1~20.04-b05
+0 🆗	spotbugs	0m 45s		branch/hadoop-project no spotbugs output file (spotbugsXml.xml)
+1 💚	shadedclient	35m 53s		branch has no errors when building and testing our client artifacts.
			_ Patch Compile Tests _
+0 🆗	mvndep	0m 39s		Maven dependency ordering for patch
+1 💚	mvninstall	1m 44s		the patch passed
+1 💚	compile	16m 39s		the patch passed with JDK Ubuntu-11.0.24+8-post-Ubuntu-1ubuntu320.04
+1 💚	javac	16m 39s		the patch passed
+1 💚	compile	16m 7s		the patch passed with JDK Private Build-1.8.0_422-8u422-b05-1~20.04-b05
+1 💚	javac	16m 7s		the patch passed
-1 ❌	blanks	0m 0s	/blanks-eol.txt	The patch has 18 line(s) that end in blanks. Use git apply --whitespace=fix <<patch_file>>. Refer https://git-scm.com/docs/git-apply
-0 ⚠️	checkstyle	4m 15s	/results-checkstyle-root.txt	root: The patch generated 2 new + 34 unchanged - 0 fixed = 36 total (was 34)
-1 ❌	mvnsite	0m 55s	/patch-mvnsite-hadoop-tools_hadoop-aws.txt	hadoop-aws in the patch failed.
+1 💚	javadoc	2m 45s		the patch passed with JDK Ubuntu-11.0.24+8-post-Ubuntu-1ubuntu320.04
+1 💚	javadoc	2m 26s		the patch passed with JDK Private Build-1.8.0_422-8u422-b05-1~20.04-b05
+0 🆗	spotbugs	0m 40s		hadoop-project has no data from spotbugs
+1 💚	shadedclient	35m 50s		patch has no errors when building and testing our client artifacts.
			_ Other Tests _
+1 💚	unit	0m 38s		hadoop-project in the patch passed.
+1 💚	unit	19m 39s		hadoop-common in the patch passed.
+1 💚	unit	3m 0s		hadoop-aws in the patch passed.
+1 💚	asflicense	1m 5s		The patch does not generate ASF License warnings.
		271m 51s

Subsystem	Report/Notes
Docker	ClientAPI=1.47 ServerAPI=1.47 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-6884/18/artifact/out/Dockerfile
GITHUB PR	#6884
Optional Tests	dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient spotbugs checkstyle codespell detsecrets xmllint markdownlint
uname	Linux 93ff913c98eb 5.15.0-124-generic #134-Ubuntu SMP Fri Sep 27 20:20:17 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev-support/bin/hadoop.sh
git revision	trunk / `1deb43a`
Default Java	Private Build-1.8.0_422-8u422-b05-1~20.04-b05
Multi-JDK versions	/usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.24+8-post-Ubuntu-1ubuntu320.04 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_422-8u422-b05-1~20.04-b05
Test Results	https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-6884/18/testReport/
Max. process+thread count	1252 (vs. ulimit of 5500)
modules	C: hadoop-project hadoop-common-project/hadoop-common hadoop-tools/hadoop-aws U: .
Console output	https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-6884/18/console
versions	git=2.25.1 maven=3.6.3 spotbugs=4.2.2
Powered by	Apache Yetus 0.14.0 https://yetus.apache.org

This message was automatically generated.

hadoop-yetus · 2024-11-01T06:46:42Z

💔 -1 overall

Vote	Subsystem	Runtime	Logfile	Comment
+0 🆗	reexec	0m 52s		Docker mode activated.
			_ Prechecks _
+1 💚	dupname	0m 0s		No case conflicting files found.
+0 🆗	codespell	0m 1s		codespell was not available.
+0 🆗	detsecrets	0m 1s		detect-secrets was not available.
+0 🆗	xmllint	0m 1s		xmllint was not available.
+0 🆗	markdownlint	0m 1s		markdownlint was not available.
+1 💚	@author	0m 0s		The patch does not contain any @author tags.
+1 💚	test4tests	0m 0s		The patch appears to include 17 new or modified test files.
			_ trunk Compile Tests _
+0 🆗	mvndep	14m 58s		Maven dependency ordering for branch
+1 💚	mvninstall	32m 26s		trunk passed
+1 💚	compile	17m 19s		trunk passed with JDK Ubuntu-11.0.24+8-post-Ubuntu-1ubuntu320.04
+1 💚	compile	16m 10s		trunk passed with JDK Private Build-1.8.0_422-8u422-b05-1~20.04-b05
+1 💚	checkstyle	4m 20s		trunk passed
+1 💚	mvnsite	3m 27s		trunk passed
+1 💚	javadoc	2m 49s		trunk passed with JDK Ubuntu-11.0.24+8-post-Ubuntu-1ubuntu320.04
+1 💚	javadoc	2m 18s		trunk passed with JDK Private Build-1.8.0_422-8u422-b05-1~20.04-b05
+0 🆗	spotbugs	0m 45s		branch/hadoop-project no spotbugs output file (spotbugsXml.xml)
+1 💚	shadedclient	35m 13s		branch has no errors when building and testing our client artifacts.
			_ Patch Compile Tests _
+0 🆗	mvndep	0m 39s		Maven dependency ordering for patch
+1 💚	mvninstall	1m 43s		the patch passed
+1 💚	compile	16m 40s		the patch passed with JDK Ubuntu-11.0.24+8-post-Ubuntu-1ubuntu320.04
+1 💚	javac	16m 40s		the patch passed
+1 💚	compile	16m 13s		the patch passed with JDK Private Build-1.8.0_422-8u422-b05-1~20.04-b05
+1 💚	javac	16m 13s		the patch passed
-1 ❌	blanks	0m 0s	/blanks-eol.txt	The patch has 1 line(s) that end in blanks. Use git apply --whitespace=fix <<patch_file>>. Refer https://git-scm.com/docs/git-apply
+1 💚	checkstyle	4m 19s		the patch passed
+1 💚	mvnsite	3m 21s		the patch passed
+1 💚	javadoc	2m 48s		the patch passed with JDK Ubuntu-11.0.24+8-post-Ubuntu-1ubuntu320.04
+1 💚	javadoc	2m 25s		the patch passed with JDK Private Build-1.8.0_422-8u422-b05-1~20.04-b05
+0 🆗	spotbugs	0m 40s		hadoop-project has no data from spotbugs
+1 💚	shadedclient	35m 41s		patch has no errors when building and testing our client artifacts.
			_ Other Tests _
+1 💚	unit	0m 37s		hadoop-project in the patch passed.
+1 💚	unit	19m 46s		hadoop-common in the patch passed.
+1 💚	unit	3m 0s		hadoop-aws in the patch passed.
+1 💚	asflicense	1m 6s		The patch does not generate ASF License warnings.
		252m 7s

Subsystem	Report/Notes
Docker	ClientAPI=1.47 ServerAPI=1.47 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-6884/19/artifact/out/Dockerfile
GITHUB PR	#6884
Optional Tests	dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient spotbugs checkstyle codespell detsecrets xmllint markdownlint
uname	Linux a49bc6f9a380 5.15.0-124-generic #134-Ubuntu SMP Fri Sep 27 20:20:17 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev-support/bin/hadoop.sh
git revision	trunk / `b196965`
Default Java	Private Build-1.8.0_422-8u422-b05-1~20.04-b05
Multi-JDK versions	/usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.24+8-post-Ubuntu-1ubuntu320.04 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_422-8u422-b05-1~20.04-b05
Test Results	https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-6884/19/testReport/
Max. process+thread count	1899 (vs. ulimit of 5500)
modules	C: hadoop-project hadoop-common-project/hadoop-common hadoop-tools/hadoop-aws U: .
Console output	https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-6884/19/console
versions	git=2.25.1 maven=3.6.3 spotbugs=4.2.2
Powered by	Apache Yetus 0.14.0 https://yetus.apache.org

This message was automatically generated.

hadoop-yetus · 2024-11-01T14:30:47Z

🎊 +1 overall

Vote	Subsystem	Runtime	Logfile	Comment
+0 🆗	reexec	0m 53s		Docker mode activated.
			_ Prechecks _
+1 💚	dupname	0m 1s		No case conflicting files found.
+0 🆗	codespell	0m 0s		codespell was not available.
+0 🆗	detsecrets	0m 0s		detect-secrets was not available.
+0 🆗	xmllint	0m 0s		xmllint was not available.
+0 🆗	markdownlint	0m 0s		markdownlint was not available.
+1 💚	@author	0m 0s		The patch does not contain any @author tags.
+1 💚	test4tests	0m 0s		The patch appears to include 17 new or modified test files.
			_ trunk Compile Tests _
+0 🆗	mvndep	14m 22s		Maven dependency ordering for branch
+1 💚	mvninstall	31m 23s		trunk passed
+1 💚	compile	17m 15s		trunk passed with JDK Ubuntu-11.0.24+8-post-Ubuntu-1ubuntu320.04
+1 💚	compile	16m 2s		trunk passed with JDK Private Build-1.8.0_422-8u422-b05-1~20.04-b05
+1 💚	checkstyle	4m 20s		trunk passed
+1 💚	mvnsite	3m 22s		trunk passed
+1 💚	javadoc	2m 54s		trunk passed with JDK Ubuntu-11.0.24+8-post-Ubuntu-1ubuntu320.04
+1 💚	javadoc	2m 25s		trunk passed with JDK Private Build-1.8.0_422-8u422-b05-1~20.04-b05
+0 🆗	spotbugs	0m 46s		branch/hadoop-project no spotbugs output file (spotbugsXml.xml)
+1 💚	shadedclient	35m 41s		branch has no errors when building and testing our client artifacts.
			_ Patch Compile Tests _
+0 🆗	mvndep	0m 38s		Maven dependency ordering for patch
+1 💚	mvninstall	1m 43s		the patch passed
+1 💚	compile	16m 39s		the patch passed with JDK Ubuntu-11.0.24+8-post-Ubuntu-1ubuntu320.04
+1 💚	javac	16m 39s		the patch passed
+1 💚	compile	16m 2s		the patch passed with JDK Private Build-1.8.0_422-8u422-b05-1~20.04-b05
+1 💚	javac	16m 2s		the patch passed
+1 💚	blanks	0m 0s		The patch has no blanks issues.
+1 💚	checkstyle	4m 16s		the patch passed
+1 💚	mvnsite	3m 18s		the patch passed
+1 💚	javadoc	2m 46s		the patch passed with JDK Ubuntu-11.0.24+8-post-Ubuntu-1ubuntu320.04
+1 💚	javadoc	2m 26s		the patch passed with JDK Private Build-1.8.0_422-8u422-b05-1~20.04-b05
+0 🆗	spotbugs	0m 40s		hadoop-project has no data from spotbugs
+1 💚	shadedclient	35m 17s		patch has no errors when building and testing our client artifacts.
			_ Other Tests _
+1 💚	unit	0m 40s		hadoop-project in the patch passed.
+1 💚	unit	19m 43s		hadoop-common in the patch passed.
+1 💚	unit	3m 0s		hadoop-aws in the patch passed.
+1 💚	asflicense	1m 4s		The patch does not generate ASF License warnings.
		250m 12s

Subsystem	Report/Notes
Docker	ClientAPI=1.47 ServerAPI=1.47 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-6884/20/artifact/out/Dockerfile
GITHUB PR	#6884
Optional Tests	dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient spotbugs checkstyle codespell detsecrets xmllint markdownlint
uname	Linux c533c8b57a58 5.15.0-124-generic #134-Ubuntu SMP Fri Sep 27 20:20:17 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev-support/bin/hadoop.sh
git revision	trunk / `59a7e05`
Default Java	Private Build-1.8.0_422-8u422-b05-1~20.04-b05
Multi-JDK versions	/usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.24+8-post-Ubuntu-1ubuntu320.04 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_422-8u422-b05-1~20.04-b05
Test Results	https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-6884/20/testReport/
Max. process+thread count	1252 (vs. ulimit of 5500)
modules	C: hadoop-project hadoop-common-project/hadoop-common hadoop-tools/hadoop-aws U: .
Console output	https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-6884/20/console
versions	git=2.25.1 maven=3.6.3 spotbugs=4.2.2
Powered by	Apache Yetus 0.14.0 https://yetus.apache.org

This message was automatically generated.

shameersss1 · 2024-11-04T06:40:09Z

@steveloughran - raised a revision by addressing the comments and checkstyle issue ifx.

steveloughran

OK. Last big bit is the exception translation.

Let's try getting this in this week!

steveloughran · 2024-11-05T18:33:18Z

hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/DefaultS3ClientFactory.java

+            configureClientBuilder(S3AsyncClient.builder(), parameters, conf, bucket)
+                .httpClientBuilder(httpClientBuilder);
+
+    // TODO: Enable multi part upload with cse once it is available.


Create a followup JIRA and reference it here "multipart upload pending with HADOOP-xyz"

steveloughran · 2024-11-05T18:36:42Z

hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/impl/S3AFileSystemHandler.java

+/**
+ * An interface that defines the contract for handling certain filesystem operations.
+ */
+public interface S3AFileSystemHandler {


steveloughran · 2024-11-05T18:37:12Z

...oop-aws/src/main/java/org/apache/hadoop/fs/s3a/impl/CSEV1CompatibleS3AFileSystemHandler.java

+
+import java.io.IOException;
+
+import org.apache.hadoop.conf.Configuration;


nit, put block below the "other" package

steveloughran · 2024-11-05T18:37:50Z

...oop-aws/src/main/java/org/apache/hadoop/fs/s3a/impl/CSEV1CompatibleS3AFileSystemHandler.java

+  public ResponseInputStream<GetObjectResponse> getObject(S3AStore store, GetObjectRequest request,
+      RequestFactory factory) throws IOException {
+    boolean isEncrypted = isObjectEncrypted(store.getOrCreateS3Client(), factory, request.key());
+    return isEncrypted ? store.getOrCreateS3Client().getObject(request)


steveloughran · 2024-11-05T18:38:57Z

...-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/impl/EncryptionS3ClientFactory.java

+import java.io.IOException;
+import java.net.URI;
+
+import org.apache.hadoop.conf.Configuration;


move block below the software. one

...ls/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AClientSideEncryptionCustom.java

steveloughran · 2024-11-05T18:56:14Z

...ls/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AClientSideEncryptionCustom.java

+    String xAttrPrefix = "header.";
+
+    // Assert KeyWrap Algo
+    assertEquals("Key wrap algo isn't same as expected", KMS_KEY_WRAP_ALGO,


assertJ assertThat

steveloughran · 2024-11-05T18:59:56Z

hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AConfiguration.java

+   * tests.
+   * @param conf Configuations
+   */
+  private static void unsetEncryption(Configuration conf) {


would it make sense to move this to S3ATestUtils and use elsewhere, unsetting all encryption options? Then use in ITestS3AClientSideEncryptionCustom

Yes make sense!

steveloughran · 2024-11-05T19:08:30Z

hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AFailureHandling.java

+    Class exceptionClass = AccessDeniedException.class;
+    if (CSEUtils.isCSEEnabled(getEncryptionAlgorithm(
+        getTestBucketName(getConfiguration()), getConfiguration()).getMethod())) {
+      exceptionClass = AWSClientIOException.class;


can you show me the full stack? 403 normally maps to AccessDeniedException, and it'd be good to keep the same. AWSClientIOException is just our "something failed" if there's nothing else

Add maybeTranslateEncryptionClientException() in ErrorTranslation to look at the exception, if the classname string value matches "software.amazon.encryption.s3.S3EncryptionClientException" then map to AccessDeniedException

call that to S3AUtils.translateException just before the // no custom handling. bit

It may seem bad, but look at maybeExtractChannelException() and other bits to see worse.

steveloughran · 2024-11-05T19:10:13Z

...op-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/impl/ITestConnectionTimeouts.java

@@ -156,7 +159,12 @@ public void testGeneratePoolTimeouts() throws Throwable {
    ContractTestUtils.createFile(fs, path, true, DATASET);
    final FileStatus st = fs.getFileStatus(path);
    try (FileSystem brittleFS = FileSystem.newInstance(fs.getUri(), conf)) {
-      intercept(ConnectTimeoutException.class, () -> {
+      Class exceptionClass = ConnectTimeoutException.class;


won't be needed once the translation is fixed.

steveloughran · 2024-11-05T19:11:51Z

let's make this the next big change, with everything else blocked before it goes in. That way, no more merge pain

hadoop-yetus · 2024-11-06T17:58:33Z

🎊 +1 overall

Vote	Subsystem	Runtime	Logfile	Comment
+0 🆗	reexec	17m 29s		Docker mode activated.
			_ Prechecks _
+1 💚	dupname	0m 1s		No case conflicting files found.
+0 🆗	codespell	0m 0s		codespell was not available.
+0 🆗	detsecrets	0m 0s		detect-secrets was not available.
+0 🆗	xmllint	0m 0s		xmllint was not available.
+0 🆗	markdownlint	0m 0s		markdownlint was not available.
+1 💚	@author	0m 0s		The patch does not contain any @author tags.
+1 💚	test4tests	0m 0s		The patch appears to include 17 new or modified test files.
			_ trunk Compile Tests _
+0 🆗	mvndep	16m 5s		Maven dependency ordering for branch
+1 💚	mvninstall	32m 32s		trunk passed
+1 💚	compile	17m 23s		trunk passed with JDK Ubuntu-11.0.24+8-post-Ubuntu-1ubuntu320.04
+1 💚	compile	16m 19s		trunk passed with JDK Private Build-1.8.0_422-8u422-b05-1~20.04-b05
+1 💚	checkstyle	4m 22s		trunk passed
+1 💚	mvnsite	3m 20s		trunk passed
+1 💚	javadoc	2m 53s		trunk passed with JDK Ubuntu-11.0.24+8-post-Ubuntu-1ubuntu320.04
+1 💚	javadoc	2m 26s		trunk passed with JDK Private Build-1.8.0_422-8u422-b05-1~20.04-b05
+0 🆗	spotbugs	0m 46s		branch/hadoop-project no spotbugs output file (spotbugsXml.xml)
+1 💚	shadedclient	36m 15s		branch has no errors when building and testing our client artifacts.
-0 ⚠️	patch	36m 42s		Used diff version of patch file. Binary files and potentially other changes not applied. Please rebase and squash commits if necessary.
			_ Patch Compile Tests _
+0 🆗	mvndep	0m 40s		Maven dependency ordering for patch
+1 💚	mvninstall	1m 45s		the patch passed
+1 💚	compile	16m 40s		the patch passed with JDK Ubuntu-11.0.24+8-post-Ubuntu-1ubuntu320.04
+1 💚	javac	16m 40s		the patch passed
+1 💚	compile	16m 10s		the patch passed with JDK Private Build-1.8.0_422-8u422-b05-1~20.04-b05
+1 💚	javac	16m 10s		the patch passed
+1 💚	blanks	0m 0s		The patch has no blanks issues.
-0 ⚠️	checkstyle	4m 15s	/results-checkstyle-root.txt	root: The patch generated 1 new + 34 unchanged - 1 fixed = 35 total (was 35)
+1 💚	mvnsite	3m 19s		the patch passed
+1 💚	javadoc	2m 48s		the patch passed with JDK Ubuntu-11.0.24+8-post-Ubuntu-1ubuntu320.04
+1 💚	javadoc	2m 25s		the patch passed with JDK Private Build-1.8.0_422-8u422-b05-1~20.04-b05
+0 🆗	spotbugs	0m 39s		hadoop-project has no data from spotbugs
+1 💚	shadedclient	37m 18s		patch has no errors when building and testing our client artifacts.
			_ Other Tests _
+1 💚	unit	0m 38s		hadoop-project in the patch passed.
+1 💚	unit	21m 13s		hadoop-common in the patch passed.
+1 💚	unit	3m 7s		hadoop-aws in the patch passed.
+1 💚	asflicense	1m 5s		The patch does not generate ASF License warnings.
		274m 20s

Subsystem	Report/Notes
Docker	ClientAPI=1.47 ServerAPI=1.47 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-6884/21/artifact/out/Dockerfile
GITHUB PR	#6884
Optional Tests	dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient spotbugs checkstyle codespell detsecrets xmllint markdownlint
uname	Linux 6dca9c8ecbd5 5.15.0-124-generic #134-Ubuntu SMP Fri Sep 27 20:20:17 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev-support/bin/hadoop.sh
git revision	trunk / `2f2790f`
Default Java	Private Build-1.8.0_422-8u422-b05-1~20.04-b05
Multi-JDK versions	/usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.24+8-post-Ubuntu-1ubuntu320.04 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_422-8u422-b05-1~20.04-b05
Test Results	https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-6884/21/testReport/
Max. process+thread count	1252 (vs. ulimit of 5500)
modules	C: hadoop-project hadoop-common-project/hadoop-common hadoop-tools/hadoop-aws U: .
Console output	https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-6884/21/console
versions	git=2.25.1 maven=3.6.3 spotbugs=4.2.2
Powered by	Apache Yetus 0.14.0 https://yetus.apache.org

This message was automatically generated.

shameersss1 · 2024-11-07T03:52:43Z

@steveloughran - Thanks a lot for the review. I have addressed your comment.

steveloughran

Thanks; I like the translation now and you can see from the tests how it makes invocation a lot more consistent. Yes -the error translation code is very complicated. But consider this: if we had not remapped all v1 SDK exceptions to IOEs before throwing them, it would have been nearly impossible to upgrade to the V2 SDK without breaking so much code downstream.

steveloughran · 2024-11-07T13:03:09Z

...-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/impl/EncryptionS3ClientFactory.java

+        keyring =
+            getKeyringProvider(cseMaterials.getCustomKeyringClassName(), cseMaterials.getConf());
+      } catch (RuntimeException e) {
+        throw new IOException("Failed to instantiate a custom keyring provider", e);


can you include the error text of e in the new IOE; it's really easy to lose the base exception in log stack chains with deep wrapping/rewrapping.

IOException("Failed to instantiate a custom keyring provider: " + e, e)

Make sense!
ack.

steveloughran · 2024-11-07T13:08:10Z

hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/impl/ErrorTranslation.java

+   * @see SdkException
+   * @see AwsServiceException
+   */
+  public static SdkException maybeExtractSdkExceptionFromEncryptionClientException(


this is a bit long. How about maybeProcessEncryptionClientException()

this says it is handled, without saying exactly what happens.

steveloughran · 2024-11-07T13:09:40Z

...-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/impl/EncryptionS3ClientFactory.java

+        return ReflectionUtils.newInstance(keyringProviderClass, conf,
+            new Class[] {Configuration.class}, conf);
+      } catch (Exception ex) {
+        throw new RuntimeException("Failed to create Keyring provider", ex);


same comment about including the wrapped exception text

shameersss1 · 2024-11-07T13:44:01Z

@steveloughran Yeah, Error translation feature make sense now. I have addressed your comments.

hadoop-yetus · 2024-11-07T17:58:31Z

🎊 +1 overall

Vote	Subsystem	Runtime	Logfile	Comment
+0 🆗	reexec	0m 55s		Docker mode activated.
			_ Prechecks _
+1 💚	dupname	0m 1s		No case conflicting files found.
+0 🆗	codespell	0m 1s		codespell was not available.
+0 🆗	detsecrets	0m 1s		detect-secrets was not available.
+0 🆗	xmllint	0m 1s		xmllint was not available.
+0 🆗	markdownlint	0m 1s		markdownlint was not available.
+1 💚	@author	0m 0s		The patch does not contain any @author tags.
+1 💚	test4tests	0m 0s		The patch appears to include 17 new or modified test files.
			_ trunk Compile Tests _
+0 🆗	mvndep	16m 19s		Maven dependency ordering for branch
+1 💚	mvninstall	31m 43s		trunk passed
+1 💚	compile	17m 22s		trunk passed with JDK Ubuntu-11.0.24+8-post-Ubuntu-1ubuntu320.04
+1 💚	compile	16m 13s		trunk passed with JDK Private Build-1.8.0_422-8u422-b05-1~20.04-b05
+1 💚	checkstyle	4m 28s		trunk passed
+1 💚	mvnsite	3m 21s		trunk passed
+1 💚	javadoc	2m 53s		trunk passed with JDK Ubuntu-11.0.24+8-post-Ubuntu-1ubuntu320.04
+1 💚	javadoc	2m 25s		trunk passed with JDK Private Build-1.8.0_422-8u422-b05-1~20.04-b05
+0 🆗	spotbugs	0m 44s		branch/hadoop-project no spotbugs output file (spotbugsXml.xml)
+1 💚	shadedclient	35m 46s		branch has no errors when building and testing our client artifacts.
-0 ⚠️	patch	36m 14s		Used diff version of patch file. Binary files and potentially other changes not applied. Please rebase and squash commits if necessary.
			_ Patch Compile Tests _
+0 🆗	mvndep	0m 41s		Maven dependency ordering for patch
+1 💚	mvninstall	1m 45s		the patch passed
+1 💚	compile	16m 42s		the patch passed with JDK Ubuntu-11.0.24+8-post-Ubuntu-1ubuntu320.04
+1 💚	javac	16m 42s		the patch passed
+1 💚	compile	16m 16s		the patch passed with JDK Private Build-1.8.0_422-8u422-b05-1~20.04-b05
+1 💚	javac	16m 16s		the patch passed
+1 💚	blanks	0m 0s		The patch has no blanks issues.
-0 ⚠️	checkstyle	4m 22s	/results-checkstyle-root.txt	root: The patch generated 1 new + 34 unchanged - 1 fixed = 35 total (was 35)
+1 💚	mvnsite	3m 20s		the patch passed
+1 💚	javadoc	2m 44s		the patch passed with JDK Ubuntu-11.0.24+8-post-Ubuntu-1ubuntu320.04
+1 💚	javadoc	2m 25s		the patch passed with JDK Private Build-1.8.0_422-8u422-b05-1~20.04-b05
+0 🆗	spotbugs	0m 40s		hadoop-project has no data from spotbugs
+1 💚	shadedclient	36m 4s		patch has no errors when building and testing our client artifacts.
			_ Other Tests _
+1 💚	unit	0m 38s		hadoop-project in the patch passed.
+1 💚	unit	20m 43s		hadoop-common in the patch passed.
+1 💚	unit	3m 7s		hadoop-aws in the patch passed.
+1 💚	asflicense	1m 7s		The patch does not generate ASF License warnings.
		255m 11s

Subsystem	Report/Notes
Docker	ClientAPI=1.47 ServerAPI=1.47 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-6884/22/artifact/out/Dockerfile
GITHUB PR	#6884
Optional Tests	dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient spotbugs checkstyle codespell detsecrets xmllint markdownlint
uname	Linux 13650f953a63 5.15.0-124-generic #134-Ubuntu SMP Fri Sep 27 20:20:17 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev-support/bin/hadoop.sh
git revision	trunk / `d4397bf`
Default Java	Private Build-1.8.0_422-8u422-b05-1~20.04-b05
Multi-JDK versions	/usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.24+8-post-Ubuntu-1ubuntu320.04 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_422-8u422-b05-1~20.04-b05
Test Results	https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-6884/22/testReport/
Max. process+thread count	3154 (vs. ulimit of 5500)
modules	C: hadoop-project hadoop-common-project/hadoop-common hadoop-tools/hadoop-aws U: .
Console output	https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-6884/22/console
versions	git=2.25.1 maven=3.6.3 spotbugs=4.2.2
Powered by	Apache Yetus 0.14.0 https://yetus.apache.org

This message was automatically generated.

shameersss1 · 2024-11-14T11:51:36Z

@steveloughran - Gentle reminder for review.

steveloughran

+1

steveloughran · 2024-11-14T13:40:22Z

ok, merged. provide a 3.4 version and I'll pull that in too.

shameersss1 · 2024-11-15T17:39:41Z

Sure, will work on branch-3.4 after HADOOP-19336

Followup to HADOOP-18708: S3A: Support S3 Client Side Encryption(CSE) (#6884) Contributed by Syed Shameerur Rahman

github-actions bot added build trunk TOOLS AWS labels Jun 12, 2024

steveloughran requested changes Jun 14, 2024

View reviewed changes

hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AFileSystem.java Outdated Show resolved Hide resolved

hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AUtils.java Outdated Show resolved Hide resolved

shameersss1 force-pushed the HADOOP-18708 branch from 7b52819 to 6464730 Compare July 8, 2024 07:37

shameersss1 requested a review from steveloughran July 8, 2024 07:37

shameersss1 force-pushed the HADOOP-18708 branch 2 times, most recently from 2593281 to bc9cfbd Compare July 8, 2024 12:50

steveloughran requested changes Jul 8, 2024

View reviewed changes

shameersss1 requested a review from steveloughran July 13, 2024 06:12

github-actions bot added the Common label Jul 13, 2024

shameersss1 force-pushed the HADOOP-18708 branch from b2207a4 to 3b31b5a Compare July 13, 2024 11:38

steveloughran mentioned this pull request Jul 15, 2024

HADOOP-19197. S3A: Support AWS KMS Encryption Context #6874

Merged

2 tasks

raphaelazzolini reviewed Jul 17, 2024

View reviewed changes

shameersss1 requested a review from raphaelazzolini July 22, 2024 04:25

raphaelazzolini approved these changes Jul 24, 2024

View reviewed changes

shameersss1 added 2 commits October 31, 2024 15:19

HADOOP-18708: Support S3 Client Side Encryption(CSE) With AWS SDK V2

882897c

Addressing review comments

1deb43a

shameersss1 force-pushed the HADOOP-18708 branch from 37eead5 to 1deb43a Compare October 31, 2024 09:54

shameersss1 requested a review from steveloughran October 31, 2024 09:56

fix checkstyle

b196965

checkstyle fix

59a7e05

steveloughran requested changes Nov 5, 2024

View reviewed changes

Fix PR review comments

2f2790f

shameersss1 requested a review from steveloughran November 7, 2024 03:51

steveloughran requested changes Nov 7, 2024

View reviewed changes

PR comments fix

d4397bf

shameersss1 requested a review from steveloughran November 7, 2024 13:43

steveloughran approved these changes Nov 14, 2024

View reviewed changes

steveloughran merged commit 2273278 into apache:trunk Nov 14, 2024
4 checks passed

steveloughran pushed a commit that referenced this pull request Dec 4, 2024

HADOOP-19349: S3A : Improve Client Side Encryption Documentation (#7191)

b55c9c7

Followup to HADOOP-18708: S3A: Support S3 Client Side Encryption(CSE) (#6884) Contributed by Syed Shameerur Rahman


		import java.io.IOException;

		import org.apache.hadoop.conf.Configuration;

HADOOP-18708: Support S3 Client Side Encryption(CSE) With AWS SDK V2 #6884

HADOOP-18708: Support S3 Client Side Encryption(CSE) With AWS SDK V2 #6884

Conversation

shameersss1 commented Jun 12, 2024 • edited Loading

Description of PR

Default Behavior

How was this patch tested?

shameersss1 commented Jun 13, 2024

steveloughran left a comment

Choose a reason for hiding this comment

steveloughran commented Jun 14, 2024

shameersss1 commented Jun 14, 2024

steveloughran commented Jun 17, 2024

steveloughran commented Jun 18, 2024

shameersss1 commented Jun 19, 2024

steveloughran commented Jun 19, 2024

shameersss1 commented Jul 8, 2024

steveloughran left a comment

Choose a reason for hiding this comment

shameersss1 commented Jul 13, 2024

steveloughran commented Jul 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shameersss1 commented Jul 22, 2024

raphaelazzolini left a comment

Choose a reason for hiding this comment

shameersss1 commented Jul 28, 2024

steveloughran commented Aug 1, 2024

steveloughran commented Oct 29, 2024

shameersss1 commented Oct 30, 2024 • edited Loading

shameersss1 commented Oct 31, 2024

hadoop-yetus commented Oct 31, 2024

hadoop-yetus commented Nov 1, 2024

hadoop-yetus commented Nov 1, 2024

shameersss1 commented Nov 4, 2024

steveloughran left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steveloughran commented Nov 5, 2024

hadoop-yetus commented Nov 6, 2024

shameersss1 commented Nov 7, 2024

steveloughran left a comment

Choose a reason for hiding this comment

steveloughran Nov 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shameersss1 commented Nov 7, 2024 • edited Loading

hadoop-yetus commented Nov 7, 2024

shameersss1 commented Nov 14, 2024

steveloughran left a comment

Choose a reason for hiding this comment

steveloughran commented Nov 14, 2024

shameersss1 commented Nov 15, 2024

shameersss1 commented Jun 12, 2024 •

edited

Loading

shameersss1 commented Oct 30, 2024 •

edited

Loading

steveloughran Nov 7, 2024 •

edited

Loading

shameersss1 commented Nov 7, 2024 •

edited

Loading